Size Change: -10 B (0%)

Total Size: 1.76 MB

_{compressed-size-action}

Flaky tests detected in f5e4bf6.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10182256602
📝 Reported issues:

• [Flaky Test] Navigates from items list to preview via TAB, and vice versa #62434 in /test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js

I think it is the right thing to do. I also did similar in this PR - #64073.

I didn't expect there would be more instances, so didn't check. 😄

From what I know, there is a kind of private setting, though there are only two keys specified (neither are sectionRootClientId):

It also looks like the settings aren't locked, just that it might strip out the private keys.

I think it might be incorrectly locked.

P.S. It seems that we have a couple of methods for making object value access private; we should probably settle on one.

I think it might be incorrectly locked.

That's interesting. I wonder why the errors happen. I guess I should revert my PR, as it might have caused some issues.

I also wonder why this is an editor setting compared to a private selector. It looks like the section client id is already derived from store state:

I also wonder why this is an editor setting compared to a private selector. It looks like the section client id is already derived from store state.

I'm unsure about that, but it seems a good candidate for the private selector. Maybe @ajlende or @scruffian can clarify the intention behind making it a setting.

I'm also concerned about how this locking works when the editor settings are updated via the reducer as it creates a new object (this is where I was looking for signs of locking):

@talldan @Mamaduka just to confirm, do you think it's the right thing to do and I should go ahead and open this for review?

@tjcafferkey, I think we might want to change how this setting is exposed. We should probably avoid adding it as a setting and create a new private selector that derives sectionRootClientId.

But let's see what others have to say.

I'm also concerned about how this locking works when the editor settings are updated via the reducer as it creates a new object (this is where I was looking for signs of locking):

I think this does work ok as the locking data is reference via a key/value to the data, so it'll be retained in the new object.

#64073 was reverted btw.

@tjcafferkey Do you have more info about how to repro the warnings/errors you're seeing?

One possible issue is that the locking of settings happens in the @wordpress/editor package, which is optional for implementors of a block editor, while lots of the unlock calls are in the @wordpress/block-editor package that they have no choice over using. The widget editors in WordPress for example don't use the editor package, so I imagine wouldn't have locked settings.

@tjcafferkey Do you have more info about how to repro the warnings/errors you're seeing?

I believe this happens when you use the useZoomOut hook in an external plugin/package. I haven't research the code extensively but I believe any packages listed here wont experience the error.

To provide testing instructions I'd likely have to use @wordpress/create-block and provide a plugin to test against unless you can think of a better way?

Edit: Apologies for the late reply here, was tied up with other things this week!

I've been AFK for a (long) while so sorry not to have inputted here sooner.

I'm unsure about that, but it seems a good candidate for the private selector. Maybe @ajlende or @scruffian can clarify the intention behind making it a setting.

My understanding (cc @scruffian and @MaggieCabrera who were here when I was away) is that sectionRootClientId is provided as a setting because it changes on a per editor basis.

The Site Editor uses the <main> tag block element as the sectionRootClientId whereas the Post Editor doesn't specify one (e.g. the editor is the root).

The sectionRootClientId is used in Zoom Out mode because we need to know where the "content" is in a given editor as that's where the "patterns as sections" need to be inserted.

Based on the above I don't think we can move the sectionRootClientId to the block editor store because then it wouldn't be configureable on a per-editor instance. Unless there's some other way I'm not aware of?

As to the locking element - I can't say, but I assume folks were trying to keep it private for now.

The Site Editor uses the

tag block element as the sectionRootClientId whereas the Post Editor doesn't specify one (e.g. the editor is the root).
Based on the above I don't think we can move the sectionRootClientId to the block editor store because then it wouldn't be configureable on a per-editor instance. Unless there's some other way I'm not aware of?

Could you have a private action for setting the sectionRootClientId (so it'd still be set via the post or site editor) and a private selector for getting it?

I also wonder why this is an editor setting compared to a private selector. It looks like the section client id is already derived from store state:

@talldan Sorry I realised I didn't fully address your point. The reason we can't put that logic directly in block editor, despite it being derived for interrogating the blocks in the editor, is that some of these blocks are specific to the WP editors (edit/post and edit/site) and thus won't exist in other contexts. That is why we made it a configurable setting as other editor instances might have different requirements.

Could you have a private action for setting the sectionRootClientId (so it'd still be set via the post or site editor) and a private selector for getting it?

Yes I was thinking about that just now. I think technically it's possible. I'm just wondering if we chose the settings root for a reason or whether it was just that alternatives weren't considered.

I'm thinking about the benefits of that approach:

• easier/clearer to implement privacy
• simpler to understand (no jumping through editor settings hoops).

Is there anything else I'm missing? I'll have a go at implementing this approach in a separate PR.

Some initial work here #64544